bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Leo Liu]

I was confused by why an eldoc-documentation-function always gets a nil
buffer-file-name in a js file in js2-mode. It turns out js2 wraps its
parsing routine in with-silent-modifications, thus all timers triggered
while parsing gets the nil value for buffer-file-name.

Any comments on the following fix? Thanks.


=== modified file 'lisp/subr.el'
--- lisp/subr.el	2014-02-12 19:40:35 +0000
+++ lisp/subr.el	2014-02-19 00:18:31 +0000
@@ -3172,21 +3172,24 @@
 Typically used around modifications of text-properties which do
 not really affect the buffer's content."
   (declare (debug t) (indent 0))
-  (let ((modified (make-symbol "modified")))
+  (let ((modified (make-symbol "modified"))
+	(mtime (make-symbol "mtime")))
     `(let* ((,modified (buffer-modified-p))
             (buffer-undo-list t)
             (inhibit-read-only t)
             (inhibit-modification-hooks t)
             deactivate-mark
-            ;; Avoid setting and removing file locks and checking
-            ;; buffer's uptodate-ness w.r.t the underlying file.
-            buffer-file-name
-            buffer-file-truename)
+            ;; Avoid checking buffer's uptodate-ness w.r.t the
+            ;; underlying file.
+            (,mtime (visited-file-modtime))
+	    ;; Avoid setting and removing file locks
+            (create-lockfiles nil))
        (unwind-protect
-           (progn
-             ,@body)
+           (progn ,@body)
          (unless ,modified
-           (restore-buffer-modified-p nil))))))
+           (restore-buffer-modified-p nil))
+	 (when (consp ,mtime)
+	   (set-visited-file-modtime ,mtime))))))
 
 (defmacro with-output-to-string (&rest body)
   "Execute BODY, return the text it sent to `standard-output', as a string."

bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Stefan Monnier]

> I was confused by why an eldoc-documentation-function always gets a nil
> buffer-file-name in a js file in js2-mode.  It turns out js2 wraps its
> parsing routine in with-silent-modifications, thus all timers triggered
> while parsing gets the nil value for buffer-file-name.

I vaguely understand what kind of problem you're talking about, but not
precisely enough to guess what kinds of fixes might make sense.

> Any comments on the following fix? Thanks.

I think I'd rather add a new inhibit-<something>, but it's hard to tell
without knowing more of the problem at hand.


        Stefan

bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Leo Liu]

On 2014-02-19 12:39 +0800, Stefan Monnier wrote:
> I vaguely understand what kind of problem you're talking about, but not
> precisely enough to guess what kinds of fixes might make sense.

I think the problem is let-binding buffer-file-name and
buffer-file-truename to nil is a really bad idea. I did a grep on
buffer-file-name in lisp/ directory and got about 2000 entries. Code at
distance may run while eval'ing the body of with-silent-modifications
and affected by the nil value of buffer-file-name etc.

>> Any comments on the following fix? Thanks.
>
> I think I'd rather add a new inhibit-<something>, but it's hard to tell
> without knowing more of the problem at hand.

So with-silent-modifications needs another way to achieve its goal. From
reading its in-code comments, it seems it wants to prevent:

        1. create and delete file locks.
        2. change file modtime which could have effect beyond the scope
           of the macro

Leo

bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Stefan Monnier]

>> I vaguely understand what kind of problem you're talking about, but not
>> precisely enough to guess what kinds of fixes might make sense.
> I think the problem is let-binding buffer-file-name and
> buffer-file-truename to nil is a really bad idea.  I did a grep on
> buffer-file-name in lisp/ directory and got about 2000 entries.  Code at
> distance may run while eval'ing the body of with-silent-modifications
> and affected by the nil value of buffer-file-name etc.

Yes, that's the general understanding I already had, but the particulars
are important as well.  Could you give details of the problem you
bump into?  Maybe a backtrace showing the sequence of calls that leads
to using buffer-file-name while it's rebound?


        Stefan

bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Leo Liu]

On 2014-02-19 22:28 +0800, Stefan Monnier wrote:
> Yes, that's the general understanding I already had, but the particulars
> are important as well.  Could you give details of the problem you
> bump into?  Maybe a backtrace showing the sequence of calls that leads
> to using buffer-file-name while it's rebound?

1. Emacs -q and eval the following code

;;;--------------------------------
(setq eldoc-idle-delay 0.2)
(add-hook 'js2-mode-hook #'bug)

(defun bug ()
  (setq-local eldoc-documentation-function #'bug-1)
  (setq js2-idle-timer-delay 0.2)
  (when buffer-file-name
    (eldoc-mode 1)))

(defun bug-1 ()
  (message "::%S::" buffer-file-name))
;;;--------------------------------

2. open a large javascript file (a few hundred lines) and switch its
   mode to js2-mode. For example

   backbone.js from git@github.com:jashkenas/backbone.git will do
   (sorry github is blocked today by my country so I cannot access it
   myself)

3. edit the file and see ::nil:: echoed

This means if function bug-1 needs buffer-file-name, directly or
indirectly, it won't work.

Leo

bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Stefan Monnier]

> (defun bug ()
>   (setq-local eldoc-documentation-function #'bug-1)
>   (setq js2-idle-timer-delay 0.2)
>   (when buffer-file-name
>     (eldoc-mode 1)))
> (defun bug-1 ()
>   (message "::%S::" buffer-file-name))

Sounds like there's a bug in js2-mode which causes it to run
eldoc-documentation-function from within a with-silent-modifications
(maybe because of a sit-for within a with-silent-modifications?).
This would be a bug regardless of whether we change
with-silent-modifications.

Again, a backtrace would be much more useful.


        Stefan

bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Leo Liu]

On 2014-02-20 02:30 +0800, Stefan Monnier wrote:
> Sounds like there's a bug in js2-mode which causes it to run
> eldoc-documentation-function from within a with-silent-modifications
> (maybe because of a sit-for within a with-silent-modifications?).
> This would be a bug regardless of whether we change
> with-silent-modifications.

Steve Yegge tried very hard to keep js2-mode performant. It seems his
strategy is to pause every few statements. (see js2-parse-statement). If
js2 find a way to disable all timers while parsing, it might for
example, cause dropping connections in rcirc. We already have Gnus doing
that.

> Again, a backtrace would be much more useful.

There is no backtrace. In my case the type inference engine for
javascript is implemented in js and the only efficient way to
communicate with it is to tell it the file and ask for information. you
could send the whole buffer over and give it a fake filename, but then
it will have to do a re-parse in full every time i.e. inefficient for
large buffers. So when buffer-file-name is nil the engine gives you no
information and thus eldoc prints nothing. This is what I meant by `not
working' without the true value of buffer-file-name.

All the above information is only remotely related because I think the
real bug is in with-silent-modifications and nxml will have similar
issues.

So to sum up:

1. buffer-file-name has many uses and disable it makes other uses
   impossible
2. what if the body in with-silent-modifications change buffers

Sorry if I fail to articulate the case. For now I have worked around it
by saving buffer-file-name to another variable.

Leo

bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Stefan Monnier]

> Steve Yegge tried very hard to keep js2-mode performant.  It seems his
> strategy is to pause every few statements. (see js2-parse-statement).
> If js2 find a way to disable all timers while parsing, it might for
> example, cause dropping connections in rcirc.  We already have Gnus
> doing that.

That doesn't change anything to the fact that calling something like
sit-for from within a with-silent-modifications is a problem, regardless
of buffer-file-name.  E.g. it will cause the process filter not to mark
the new text for highlighting, so it can screw up *compilation*.

>> Again, a backtrace would be much more useful.
> There is no backtrace.

Of course there is.  I know there is no error signaled, but you can
still get a backtrace by calling (backtrace), or (debug).


        Stefan

bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Leo Liu]

On 2014-02-20 12:57 +0800, Stefan Monnier wrote:
> That doesn't change anything to the fact that calling something like
> sit-for from within a with-silent-modifications is a problem, regardless
> of buffer-file-name.  E.g. it will cause the process filter not to mark
> the new text for highlighting, so it can screw up *compilation*.

I see. Could you help document the delicacy of
with-silent-modifications, or people will not correctly use it?

>>> Again, a backtrace would be much more useful.
>> There is no backtrace.
>
> Of course there is.  I know there is no error signaled, but you can
> still get a backtrace by calling (backtrace), or (debug).

I see and sorry I misunderstood.

Since input-pending-p is changed in 24.4 not to run timers by default,
the backtrace is obtained in 24.3 and as expected:

  ......
  timer-event-handler([t 0 0 200000 t #[...] nil idle 0])
  input-pending-p()
  js2-parse-statement()
  js2-parse-function-body([cl-struct-js2-function-node 108 3537 1 nil nil nil ((once . [cl-struct-js2-symbol 121 "once" [cl-struct-js2-name-node 39 0 4 nil [cl-struct-js2-var-init-node 121 4 106 nil [cl-struct-js2-var-decl-node 121 0 110 nil [cl-struct-js2-expr-stmt-node 132 120 111 nil [cl-struct-js2-block-node 128 3571 1 nil nil ...] #6] (#5) 121] #4 [cl-struct-js2-call-node 38 7 99 nil #5 [cl-struct-js2-prop-get-node 33 0 6 nil #6 3703 [cl-struct-js2-name-node 39 0 1 nil #7 "_" nil] [cl-struct-js2-name-node 39 2 4 nil #7 "once" nil]] ([cl-struct-js2-function-node 108 7 91 nil #6 nil nil #0 nil nil nil nil 0 nil nil 0 FUNCTION FUNCTION_EXPRESSION nil nil nil [cl-struct-js2-block-node 128 11 80 nil #8 ...] 8 9 t t nil nil]) 6 98]] "once" nil]]) (self . [cl-struct-js2-symbol 121 "self" [cl-struct-js2-name-node 39 0 4 nil [cl-struct-js2-var-init-node 121 4 11 nil [cl-struct-js2-var-decl-node 121 0 15 nil [cl-struct-js2-expr-stmt-node 132 97 16 nil [cl-struct-js2-block-node 128 3571 1 nil nil ...] #6] (#5) 121] #4 [cl-struct-js2-keyword-node 43 7 4 nil #5]] "self" nil]]) (context . [cl-struct-js2-symbol 86 "context" [cl-struct-js2-name-node 39 25 7 nil #0 "context" nil]]) (callback . [cl-struct-js2-symbol 86 "callback" [cl-struct-js2-name-node 39 15 8 nil #0 "callback" nil]]) (name . [cl-struct-js2-symbol 86 "name" [cl-struct-js2-name-node 39 9 4 nil #0 "name" nil]])) nil nil nil nil nil 0 nil nil 0 FUNCTION FUNCTION_EXPRESSION nil ([cl-struct-js2-name-node 39 9 4 nil #0 "name" nil] [cl-struct-js2-name-node 39 15 8 nil #0 "callback" nil] [cl-struct-js2-name-node 39 25 7 nil #0 "context" nil]) nil nil 8 32 t nil nil nil])
  js2-parse-function(FUNCTION_EXPRESSION 3537 nil nil)
  js2-parse-function-expr()
  js2-parse-primary-expr()
  js2-parse-member-expr(t)
  js2-parse-unary-expr()
  js2-parse-mul-expr()
  js2-parse-add-expr()
  js2-parse-shift-expr()
  js2-parse-rel-expr()
  js2-parse-eq-expr()
  js2-parse-bit-and-expr()
  ......

nxml has both input-pending-p and sit-for, see
rng-validate-while-idle-continue-p in rng-valid.el

>         Stefan

Is there something better than let-binding buffer-file-name and
buffer-file-truename?

Leo

bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Stefan Monnier]

>> That doesn't change anything to the fact that calling something like
>> sit-for from within a with-silent-modifications is a problem, regardless
>> of buffer-file-name.  E.g. it will cause the process filter not to mark
>> the new text for highlighting, so it can screw up *compilation*.
> I see.  Could you help document the delicacy of
> with-silent-modifications, or people will not correctly use it?

Actually, the issue is not with with-silent-modifications but with
sit-for and other yielding functions: you should be careful to call them
from within the "plainest" context possible, since any dynamic-binding
(or other temporary global change, such as moving point) currently active
will then leak to any timer/filter/sentinel/redisplay that might be run
at that point.

>>>> Again, a backtrace would be much more useful.
>>> There is no backtrace.
>> Of course there is.  I know there is no error signaled, but you can
>> still get a backtrace by calling (backtrace), or (debug).
> Since input-pending-p is changed in 24.4 not to run timers by default,
> the backtrace is obtained in 24.3 and as expected:

Ah, right, input-pending-p shouldn't be a yield point, which we recently
fixed, so it should be OK to call input-pending-p from "anywhere".

Can you reproduce the problem in 24.4 (and show the backtrace)?

> nxml has both input-pending-p and sit-for, see
> rng-validate-while-idle-continue-p in rng-valid.el

Indeed, I see
		   (with-silent-modifications
		     (rng-do-some-validation-1 continue-p-function))))

where continue-p-function can be rng-validate-while-idle-continue-p
which calls sit-for (to cause a redisplay).  So we have a similar bug there.

> Is there something better than let-binding buffer-file-name and
> buffer-file-truename?

I have the patch below for 24.5 which makes it unnecessary for
with-silent-modifications to bind buffer-file-name and
buffer-file-truename (by strengthening the meaning of
inhibit-modification-hooks), but that doesn't magically fix the above
problems :-(


        Stefan


=== modified file 'lisp/subr.el'
--- lisp/subr.el	2014-02-12 19:40:35 +0000
+++ lisp/subr.el	2014-02-19 15:04:50 +0000
@@ -3176,12 +3176,7 @@
     `(let* ((,modified (buffer-modified-p))
             (buffer-undo-list t)
             (inhibit-read-only t)
-            (inhibit-modification-hooks t)
-            deactivate-mark
-            ;; Avoid setting and removing file locks and checking
-            ;; buffer's uptodate-ness w.r.t the underlying file.
-            buffer-file-name
-            buffer-file-truename)
+            (inhibit-modification-hooks t))
        (unwind-protect
            (progn
              ,@body)

=== modified file 'src/insdel.c'
--- src/insdel.c	2014-01-01 17:44:48 +0000
+++ src/insdel.c	2014-02-19 15:03:17 +0000
@@ -1829,6 +1829,9 @@
   else
     base_buffer = current_buffer;
 
+  if (inhibit_modification_hooks)
+    return;
+
 #ifdef CLASH_DETECTION
   if (!NILP (BVAR (base_buffer, file_truename))
       /* Make binding buffer-file-name to nil effective.  */
@@ -1848,7 +1851,6 @@
   /* If `select-active-regions' is non-nil, save the region text.  */
   /* FIXME: Move this to Elisp (via before-change-functions).  */
   if (!NILP (BVAR (current_buffer, mark_active))
-      && !inhibit_modification_hooks
       && XMARKER (BVAR (current_buffer, mark))->buffer
       && NILP (Vsaved_region_selection)
       && (EQ (Vselect_active_regions, Qonly)
@@ -1959,9 +1961,6 @@
   ptrdiff_t count = SPECPDL_INDEX ();
   struct rvoe_arg rvoe_arg;
 
-  if (inhibit_modification_hooks)
-    return;
-
   start = make_number (start_int);
   end = make_number (end_int);
   preserve_marker = Qnil;

bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Leo Liu]

On 2014-02-20 22:01 +0800, Stefan Monnier wrote:
> Actually, the issue is not with with-silent-modifications but with
> sit-for and other yielding functions: you should be careful to call them
> from within the "plainest" context possible, since any dynamic-binding
> (or other temporary global change, such as moving point) currently active
> will then leak to any timer/filter/sentinel/redisplay that might be run
> at that point.

OK for now.

>> Since input-pending-p is changed in 24.4 not to run timers by default,
>> the backtrace is obtained in 24.3 and as expected:
>
> Ah, right, input-pending-p shouldn't be a yield point, which we recently
> fixed, so it should be OK to call input-pending-p from "anywhere".
>
> Can you reproduce the problem in 24.4 (and show the backtrace)?

No. I tried but cannot get any backtrace in 24.4 so input-pending-p is
behaving.

>> nxml has both input-pending-p and sit-for, see
>> rng-validate-while-idle-continue-p in rng-valid.el
>
> Indeed, I see
> 		   (with-silent-modifications
> 		     (rng-do-some-validation-1 continue-p-function))))
>
> where continue-p-function can be rng-validate-while-idle-continue-p
> which calls sit-for (to cause a redisplay).  So we have a similar bug there.

How about replace (sit-for 0) with (not (input-pending-p)) in
rng-validate-while-idle-continue-p?

>> Is there something better than let-binding buffer-file-name and
>> buffer-file-truename?
>
> I have the patch below for 24.5 which makes it unnecessary for
> with-silent-modifications to bind buffer-file-name and
> buffer-file-truename (by strengthening the meaning of
> inhibit-modification-hooks), but that doesn't magically fix the above
> problems :-(
>
>
>         Stefan

Thanks for the fix.

Leo

bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Stefan Monnier]

>> Can you reproduce the problem in 24.4 (and show the backtrace)?
> No.  I tried but cannot get any backtrace in 24.4 so input-pending-p is
> behaving.

Does that mean that the original problem (interaction with js2-mode) you
reported is only present in 24.3 and not in 24.4 (your original email
says "24.3.50", which makes me believe the problem also applies to
24.4)?

> How about replace (sit-for 0) with (not (input-pending-p)) in
> rng-validate-while-idle-continue-p?

No, this call to sit-for is performed specifically to refresh the
display, so input-pending-p is not an alternative.


        Stefan

bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Leo Liu]

On 2014-02-21 01:14 +0800, Stefan Monnier wrote:
> Does that mean that the original problem (interaction with js2-mode) you
> reported is only present in 24.3 and not in 24.4 (your original email
> says "24.3.50", which makes me believe the problem also applies to
> 24.4)?

I believed it too but I was too focused on the issues of
with-silent-modifications and thus the 24.3.50 tag. Now I cannot
reproduce the problem on 24.3.50 any more.

I might have some missteps in the initial report. I discovered the
problem while running 24.3 on my laptop then ssh'd to a centos 6 server
where there is daily build of emacs and "saw" the same issue there.

>> How about replace (sit-for 0) with (not (input-pending-p)) in
>> rng-validate-while-idle-continue-p?
>
> No, this call to sit-for is performed specifically to refresh the
> display, so input-pending-p is not an alternative.

OK.

Leo

bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Lars Ingebrigtsen]

Leo Liu <sdl.web@gmail.com> writes:

> On 2014-02-21 01:14 +0800, Stefan Monnier wrote:
>> Does that mean that the original problem (interaction with js2-mode) you
>> reported is only present in 24.3 and not in 24.4 (your original email
>> says "24.3.50", which makes me believe the problem also applies to
>> 24.4)?
>
> I believed it too but I was too focused on the issues of
> with-silent-modifications and thus the 24.3.50 tag. Now I cannot
> reproduce the problem on 24.3.50 any more.

It sounds like this problem has gone away?  So I'm closing this bug
report.  If it still persists, please reopen.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no