bug#25132: 26.0.50; emacs hangs when loading org file with python source blocks

bug#25132: 26.0.50; emacs hangs when loading org file with python source blocks

[Glenn Morris]

David Dynerman wrote:

> The bug does NOT occur with org 8.2.10.

Since that's the version included with Emacs, I'm confused as to why
you've been encouraged to report this to bug-gnu-emacs.

bug#25132: 26.0.50; emacs hangs when loading org file with python source blocks

[Clément Pit--Claudel]

[-- Attachment #1.1.1: Type: text/plain, Size: 720 bytes --]

On 2016-12-07 21:08, Glenn Morris wrote:
> David Dynerman wrote:
>> The bug does NOT occur with org 8.2.10.
> 
> Since that's the version included with Emacs, I'm confused as to why
> you've been encouraged to report this to bug-gnu-emacs.

I can reproduce the issue in emacs -Q on master; hence my suggestion to report it here.  Glenn, can you try running the following after downloading the attached file?

   emacs -Q --eval '(setq debug-on-signal t org-src-fontify-natively t)' test.org 

It hangs reproducibly for me.  No idea why the OP can't reproduce it (David, are you sure it doesn't occur with org 8.2.10? Could it be that org-src-fontify-natively isn't enabled by default in 8.2.10?)

Clément.

[-- Attachment #1.1.2: test.org --]
[-- Type: application/vnd.lotus-organizer, Size: 171 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#25132: 26.0.50; emacs hangs when loading org file with python source blocks

[David Dynerman]

Dear Glenn + bug-gnu-emacs,

Did you try the steps to reproduce? Indeed Clément was right! The bug also reliably reproduces also with org 8.2.10, if you additionally set org-src-fontify-natively to t.

Please let me know if you need any more information for the bug report,
David

Clément Pit--Claudel <clement.pit@gmail.com> writes:

> On 2016-12-07 21:08, Glenn Morris wrote:
>> David Dynerman wrote:
>>> The bug does NOT occur with org 8.2.10.
>> 
>> Since that's the version included with Emacs, I'm confused as to why
>> you've been encouraged to report this to bug-gnu-emacs.
>
> I can reproduce the issue in emacs -Q on master; hence my suggestion to report it here.  Glenn, can you try running the following after downloading the attached file?
>
>    emacs -Q --eval '(setq debug-on-signal t org-src-fontify-natively t)' test.org 
>
> It hangs reproducibly for me.  No idea why the OP can't reproduce it (David, are you sure it doesn't occur with org 8.2.10? Could it be that org-src-fontify-natively isn't enabled by default in 8.2.10?)
>
> Clément.

bug#25132: 26.0.50; emacs hangs when loading org file with python source blocks

[npostavs]

tags 25132 confirmed
quit

The problem is that org updates its temporary fontification buffer from
its fontify rules which are called by jit-lock-function, which means
that inhibit-modification-hooks is bound to t.  Therefore, when
org-src-font-lock-fontify-block calls delete-region to remove leftover text from
the previous source block fontification, the `before-change-functions'
are not run.  In this case `syntax-ppss-flush-cache' is the important
function that doesn't get run, so `syntax-propertize--done' is still set
from before and messes up python.el's fontification routines.

  org-src-font-lock-fontify-block(#("python" 0 6 (fontified t)) 19 65)
  org-fontify-meta-lines-and-blocks-1(172)
  org-fontify-meta-lines-and-blocks(172)
  font-lock-fontify-keywords-region(1 172 nil)
  font-lock-default-fontify-region(1 172 nil)
  font-lock-fontify-region(1 172)
  ...
  jit-lock--run-functions(1 172)
  jit-lock-fontify-now(1 501)
  jit-lock-function(1)
  redisplay_internal\ \(C\ function\)()
  redisplay()
  sit-for(2)
  execute-extended-command(nil "25132-test" "25")
  funcall-interactively(execute-extended-command nil "25132-test" "25")
  call-interactively(execute-extended-command nil nil)
  command-execute(execute-extended-command)

(defun org-src-font-lock-fontify-block (lang start end)
  ...
	  (with-current-buffer
	      (get-buffer-create
	       (concat " org-src-fontification:" (symbol-name lang-mode)))
            (delete-region (point-min) (point-max)) ;<-------------- `syntax-propertize--done' not reset here!
            (insert string " ") ;; so there's a final property change
	    (unless (eq major-mode lang-mode) (funcall lang-mode))
            (org-font-lock-ensure)
            ...)
  ...)

(defun jit-lock-function (start)
  ...
  (jit-lock-fontify-now start (+ start jit-lock-chunk-size))
  ...)

(defun jit-lock-fontify-now (&optional start end)
  "Fontify current buffer from START to END.
Defaults to the whole buffer.  END can be out of bounds."
  (with-buffer-prepared-for-jit-lock
    ...))

  (defmacro with-buffer-prepared-for-jit-lock (&rest body)
    "Execute BODY in current buffer, overriding several variables.
Preserves the `buffer-modified-p' state of the current buffer."
    (declare (debug t))
    `(let ((inhibit-point-motion-hooks t))
       (with-silent-modifications ; <------ binds inhibit-modification-hooks to t
         ,@body)))

bug#25132: 26.0.50; emacs hangs when loading org file with python source blocks

[npostavs]

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

tags 25132 patch
quit

npostavs@users.sourceforge.net writes:
> The problem is that org updates its temporary fontification buffer from
> its fontify rules which are called by jit-lock-function, which means
> that inhibit-modification-hooks is bound to t.  Therefore, when
> org-src-font-lock-fontify-block calls delete-region to remove leftover text from
> the previous source block fontification, the `before-change-functions'
> are not run.  In this case `syntax-ppss-flush-cache' is the important
> function that doesn't get run, so `syntax-propertize--done' is still set
> from before and messes up python.el's fontification routines.

I think with-silent-modifications should let-bind
inhibit-modification-hooks buffer locally:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch for master --]
[-- Type: text/x-diff, Size: 1171 bytes --]

From da4f1c0338b2b98f97a553568c4b80872484ee97 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 7 Jan 2017 15:47:37 -0500
Subject: [PATCH v1] Inhibit modification hooks buffer locally

`with-silent-modifications' let-binds `inhibit-modification-hooks' to t
globally.  So modifications to other buffers don't trigger modication
hooks.  This causes unexpected results when functions called from
`jit-lock-function' use temporary buffers and modifies them (Bug#25132).

* lisp/subr.el (with-silent-modifications): Bind
inhibit-modification-hooks buffer locally.
---
 lisp/subr.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index 5377416..fe20d68 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3298,7 +3298,8 @@ with-silent-modifications
     `(let* ((,modified (buffer-modified-p))
             (buffer-undo-list t)
             (inhibit-read-only t)
-            (inhibit-modification-hooks t))
+            (inhibit-modification-hooks
+             (progn (make-local-variable 'inhibit-modification-hooks) t)))
        (unwind-protect
            (progn
              ,@body)
-- 
2.9.3


[-- Attachment #3: Type: text/plain, Size: 219 bytes --]


Perhaps the other variables it binds should be buffer local as well?

This bug is new in Emacs 25.1, but changing with-silent-modifications is
a bit risky.  Therefore, I propose the following for the emacs-25
branch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: patch for emacs-25 --]
[-- Type: text/x-diff, Size: 1262 bytes --]

From 338aa0c37eba0401616e8e02f0143a57edffd486 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 7 Jan 2017 16:05:19 -0500
Subject: [PATCH v1] Call modification hooks in org-src fontify buffers

* lisp/org/org-src.el (org-src-font-lock-fontify-block): Let-bind
`inhibit-modification-hooks' to nil, since this function can be called
from jit-lock-function which binds that variable to t (Bug#25132).
---
 lisp/org/org-src.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/org/org-src.el b/lisp/org/org-src.el
index d01f108..9b66907 100644
--- a/lisp/org/org-src.el
+++ b/lisp/org/org-src.el
@@ -913,8 +913,9 @@ org-src-font-lock-fontify-block
 	  (with-current-buffer
 	      (get-buffer-create
 	       (concat " org-src-fontification:" (symbol-name lang-mode)))
-	    (delete-region (point-min) (point-max))
-	    (insert string " ") ;; so there's a final property change
+	    (let ((inhibit-modification-hooks nil)) ; Avoid Bug#25132.
+              (delete-region (point-min) (point-max))
+              (insert string " ")) ;; so there's a final property change
 	    (unless (eq major-mode lang-mode) (funcall lang-mode))
             (org-font-lock-ensure)
 	    (setq pos (point-min))
-- 
2.9.3

bug#25132: 26.0.50; emacs hangs when loading org file with python source blocks

[Dmitry Gutov]

On 08.01.2017 00:20, npostavs@users.sourceforge.net wrote:
> -            (inhibit-modification-hooks t))
> +            (inhibit-modification-hooks
> +             (progn (make-local-variable 'inhibit-modification-hooks) t)))

Are we not worried that inhibit-modificaiton-hooks will become 
buffer-local even after control flow leaves this let*?

If we are not, why not make inhibit-modification-hooks always 
buffer-local instead?

bug#25132: 26.0.50; emacs hangs when loading org file with python source blocks

[npostavs]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 08.01.2017 00:20, npostavs@users.sourceforge.net wrote:
>> -            (inhibit-modification-hooks t))
>> +            (inhibit-modification-hooks
>> +             (progn (make-local-variable 'inhibit-modification-hooks) t)))
>
> Are we not worried that inhibit-modificaiton-hooks will become
> buffer-local even after control flow leaves this let*?

My feeling is that inhibit-modification-hooks should usually be buffer
local anyway.

> If we are not, why not make inhibit-modification-hooks always
> buffer-local instead?

It would have to be in addition to, because even after doing
(make-variable-buffer-local 'var), (let ((var 'foo))...) still makes a
global binding.  `make-variable-buffer-local' only has effect for
`setq', which I think will hardly ever happen for
`inhibit-modification-hooks'.

Actually, I just grepped for inhibit-modification-hooks and the only
non-let I found is this:

(defun read-passwd (prompt &optional confirm default)
   ...
      (minibuffer-with-setup-hook
          (lambda ()
             ...
             (setq-local inhibit-modification-hooks nil) ;bug#15501.
             ...))
   ...)

bug#25132: 26.0.50; emacs hangs when loading org file with python source blocks

[Clément Pit--Claudel]

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

On 2017-01-19 19:52, npostavs@users.sourceforge.net wrote:
> because even after doing (make-variable-buffer-local 'var), (let
> ((var 'foo))...) still makes a global binding.
> `make-variable-buffer-local' only has effect for `setq', which I
> think will hardly ever happen for `inhibit-modification-hooks'.

On 2017-01-19 19:52, npostavs@users.sourceforge.net wrote:
> because even after doing (make-variable-buffer-local 'var), (let
> ((var 'foo))...) still makes a global binding.
> `make-variable-buffer-local' only has effect for `setq', which I
> think will hardly ever happen for `inhibit-modification-hooks'.

Hi Noam,

Can you explain a bit more? I'm not sure what you meant.

I tried the following to illustrate your point:

(defvar aa 0)

(with-temp-buffer
  (setq-local aa 1)
  (let ((b1 (current-buffer)))
    (with-temp-buffer
      (let ((aa 2))
        (message "In b2: %S" aa)
        (with-current-buffer b1
          (message "In b1: %S" aa))))))

Clément.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#25132: 26.0.50; emacs hangs when loading org file with python source blocks

[npostavs]

Clément Pit--Claudel <clement.pit@gmail.com> writes:

> On 2017-01-19 19:52, npostavs@users.sourceforge.net wrote:
>> because even after doing (make-variable-buffer-local 'var), (let
>> ((var 'foo))...) still makes a global binding.
>> `make-variable-buffer-local' only has effect for `setq', which I
>> think will hardly ever happen for `inhibit-modification-hooks'.
>
> On 2017-01-19 19:52, npostavs@users.sourceforge.net wrote:
>> because even after doing (make-variable-buffer-local 'var), (let
>> ((var 'foo))...) still makes a global binding.
>> `make-variable-buffer-local' only has effect for `setq', which I
>> think will hardly ever happen for `inhibit-modification-hooks'.
>
> Hi Noam,
>
> Can you explain a bit more? I'm not sure what you meant.
>
> I tried the following to illustrate your point:
>
> (defvar aa 0)
>
> (with-temp-buffer
>   (setq-local aa 1)
>   (let ((b1 (current-buffer)))
>     (with-temp-buffer
>       (let ((aa 2))
>         (message "In b2: %S" aa)
>         (with-current-buffer b1
>           (message "In b1: %S" aa))))))

My point was that the setq-local (or make-local-variable) is required
and that defvar-local (or make-variable-buffer-local) is not enough.

Compare:

(defvar-local bb 0)

(with-temp-buffer
  (let ((b1 (current-buffer)))
    (with-temp-buffer
      (let ((bb 2))
        (message "In b2: %S" bb)
        (with-current-buffer b1
          (message "In b1: %S" bb))))))

bug#25132: 26.0.50; emacs hangs when loading org file with python source blocks

[Dmitry Gutov]

On 20.01.2017 03:52, npostavs@users.sourceforge.net wrote:

> My feeling is that inhibit-modification-hooks should usually be buffer
> local anyway.

Maybe you're right.

inhibit-read-only, bound nearby, seems to be in the same situation.

>> If we are not, why not make inhibit-modification-hooks always
>> buffer-local instead?
>
> It would have to be in addition to, because even after doing
> (make-variable-buffer-local 'var), (let ((var 'foo))...) still makes a
> global binding.  `make-variable-buffer-local' only has effect for
> `setq', which I think will hardly ever happen for
> `inhibit-modification-hooks'.

You're right, and that sounds a little too complicated for my taste.

So, personally, I'd try to fix the particular instance first. Switching 
buffers inside with-silent-modifications is not a very common usage, I 
think.

Maybe org-src should itself let-bind the aforementioned variable(s) 
where it visits other buffers.

Up to you, of course, since you've already been given the go-ahead for 
the proposed fix.

bug#25132: 26.0.50; emacs hangs when loading org file with python source blocks

[npostavs]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 20.01.2017 03:52, npostavs@users.sourceforge.net wrote:
>
>> My feeling is that inhibit-modification-hooks should usually be buffer
>> local anyway.
>
> Maybe you're right.
>
> inhibit-read-only, bound nearby, seems to be in the same situation.
>
>>> If we are not, why not make inhibit-modification-hooks always
>>> buffer-local instead?
>>
>> It would have to be in addition to, because even after doing
>> (make-variable-buffer-local 'var), (let ((var 'foo))...) still makes a
>> global binding.  `make-variable-buffer-local' only has effect for
>> `setq', which I think will hardly ever happen for
>> `inhibit-modification-hooks'.
>
> You're right, and that sounds a little too complicated for my taste.
>
> So, personally, I'd try to fix the particular instance
> first. Switching buffers inside with-silent-modifications is not a
> very common usage, I think.
>
> Maybe org-src should itself let-bind the aforementioned variable(s)
> where it visits other buffers.

Yeah, that works, and is my proposal for emacs-25, but I'm still leaning
towards solving this more broadly in with-silent-modifications, probably
also add a mention about this to the inhibit-modification-hooks
docstring.

I think doing the same to inhibit-read-only isn't worth the trouble
because if it happens to be let-bound to t in a buffer where it wasn't
"supposed" to be, the worst that happens is that an error *isn't*
thrown.

bug#25132: 26.0.50; emacs hangs when loading org file with python source blocks

[npostavs]

tags 25132 fixed
close 25132 25.2
quit

npostavs@users.sourceforge.net writes:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> So, personally, I'd try to fix the particular instance
>> first. Switching buffers inside with-silent-modifications is not a
>> very common usage, I think.
>>
>> Maybe org-src should itself let-bind the aforementioned variable(s)
>> where it visits other buffers.
>
> Yeah, that works, and is my proposal for emacs-25, but I'm still leaning
> towards solving this more broadly in with-silent-modifications, probably
> also add a mention about this to the inhibit-modification-hooks
> docstring.

I changed my mind.  Bug#25561 reminded me about the "Making <var> local
to <buffer> while let-bound!" message.  My change to
`with-silent-modifications' would trigger that on any nested invocations
of `with-silent-modifications' which seems more likely to happen than
switching buffers.

I've pushed the simpler let-bind in org-src solution [1: ae8264c] to
emacs-25.

1: 2017-01-29 11:01:32 -0500 ae8264c5cccf19d5b25a340a605bf2f07de1577e
  Call modification hooks in org-src fontify buffers