bug#41441: 26.3; mhtml misbehaving font-lock extend region can cause Emacs to hang

bug#41441: 26.3; mhtml misbehaving font-lock extend region can cause Emacs to hang

[Anders Lindgren]

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

Hi!

The font-lock system provides a system for major and minor modes to extend
the region that is being highlighted.

In Mhtml mode, the function `mhtml--extend-font-lock-region` sometimes
reduce the size of the region. If this is combined with, say, a minor mode
that extends the region then Emacs could hang.

Steps to repeat:

Create a file, test.html, with the following content:

---------
alpha

beta
---------

Evaluate:

(let ((font-lock-beg 7)
      (font-lock-end 13))
  (mhtml--extend-font-lock-region)
  (cons font-lock-beg font-lock-end))

The 7 and 13 corresponds to a region that includes the last two lines. The
expression returns (8 . 13) representing a region that only include the
last line. If this is combined with another function that extends the
region to include line 2, Emacs hangs.

I suggest that:

* Modify the documentation of the variable
`font-lock-extend-region-functions` so that it's clear that the region
should not shrink.

* Add a check in the function `font-lock-default-fontify-region` to prevent
the region from shrinking even if a function tries to do so. (By ensuring
that the region only extends and never shrinks, the function will always
terminate.)

* Fix the problem in `mhtml--extend-font-lock-region`.

One minor mode that, when enabled, would cause Emacs to hang with the above
buffer is https://github.com/Lindydancer/char-font-lock (this package
highlights incorrect whitespace).

    -- Anders Lindgren




1) The function `mhtml--extend-font-lock-region should be modified so that
it never shrinks the region.


In GNU Emacs 26.3 (build 1, x86_64-apple-darwin14.5.0, NS appkit-1348.17
Version 10.10.5 (Build 14F2511))
 of 2019-09-02 built on builder10-10.porkrind.org
Windowing system distributor 'Apple', version 10.3.1348
Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
(New file)
funcall-interactively: End of buffer
Saving file /Users/anders/emacs/src/foreach/foo.html...
Wrote /Users/anders/emacs/src/foreach/foo.html
Char: C-j (10, #o12, #xa) point=7 of 12 (50%) column=0
Quit
Entering debugger...
Back to top level
(8 . 13) [2 times]
Configured using:
 'configure --with-ns '--enable-locallisppath=/Library/Application
 Support/Emacs/${version}/site-lisp:/Library/Application
 Support/Emacs/site-lisp' --with-modules'

Configured features:
NOTIFY ACL GNUTLS LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS MODULES THREADS

Important settings:
  value of $LC_CTYPE: UTF-8
  value of $LANG: en_SE.UTF-8
  locale-coding-system: utf-8-unix

Major mode: HTML+

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
format-spec rfc822 mml mml-sec password-cache epa derived epg epg-config
gnus-util rmail rmail-loaddefs mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils cl-print debug help-fns
radix-tree help-mode vc-dispatcher vc-svn mhtml-mode css-mode smie color
js advice json map imenu thingatpt cc-mode cc-fonts easymenu cc-guess
cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs sgml-mode
seq byte-opt gv bytecomp byte-compile cconv dom cl-loaddefs cl-lib
elec-pair time-date tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/ns-win ns-win ucs-normalize mule-util
term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads kqueue cocoa ns multi-tty make-network-process emacs)

Memory information:
((conses 16 244001 21210)
 (symbols 48 23926 1)
 (miscs 40 81 268)
 (strings 32 43844 1478)
 (string-bytes 1 1234265)
 (vectors 16 39716)
 (vector-slots 8 775521 10394)
 (floats 8 209 245)
 (intervals 56 230 0)
 (buffers 992 14))

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

bug#41441: 26.3; mhtml misbehaving font-lock extend region can cause Emacs to hang

[Tom Tromey]

Anders> * Fix the problem in `mhtml--extend-font-lock-region`.

It's not entirely clear to me that (1) this is a bug, or (2) that it can
be changed.

My recollection is that mhtml mode has to shrink the region in some
cases, because we don't want font-locking to extend beyond the end of a
sub-mode.
           
For example consider things like

<script>some js here;</script><p>more html here</p>

Here, we want to font-lock the body of the script using one set of
rules, and the rest with another set.

Looking at mhtml--submode-fontify-region, though, I wonder if maybe
region extension isn't needed at all, since that function seems to
handle sub-mode region boundaries.  So I guess that is one
experiment that could be done.

Anders> One minor mode that, when enabled, would cause Emacs to hang
Anders> with the above buffer is
Anders> https://github.com/Lindydancer/char-font-lock (this package
Anders> highlights incorrect whitespace).

Maybe this mode could be changed instead.

Tom

bug#41441: 26.3; mhtml misbehaving font-lock extend region can cause Emacs to hang

[Anders Lindgren]

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

Hi!

To me it's obviously a bug. First, the name of the function contains
"extend", not "change". Secondly, the loop in
font-lock-default-fontify-region is written so that it loops until all
functions in font-lock-extend-region-functions are satisfied (which is also
documented). To me, when code is written this way it's clear that monotonic
growth of the region is assumed (even though that isn't explicitly stated
in the documentation).

Anyway, I took a look at the mhtml code today. This is from
mhtml--extend-font-lock-region:

      (goto-char font-lock-beg)
      (unless (eobp)
        (forward-char))
      (setq font-lock-beg (previous-single-property-change
                           (point) 'mhtml-submode nil
                           (line-beginning-position)))

Without having a deep knowledge of this, I don't think this do the correct
thing when font-lock-beg is at the end of a line (as it is when the line is
empty). The problem is that the code move forward one character (the
newline) and then search backward with line-beginning-position (i.e. the
beginning of the line following the original font-lock-beg) as limit,
effectively shrinking the region.


On Mon, May 25, 2020 at 6:11 PM Tom Tromey <tom@tromey.com> wrote:

> Anders> * Fix the problem in `mhtml--extend-font-lock-region`.
>
> It's not entirely clear to me that (1) this is a bug, or (2) that it can
> be changed.
>
> My recollection is that mhtml mode has to shrink the region in some
> cases, because we don't want font-locking to extend beyond the end of a
> sub-mode.
>
> For example consider things like
>
> <script>some js here;</script><p>more html here</p>
>
> Here, we want to font-lock the body of the script using one set of
> rules, and the rest with another set.
>
> Looking at mhtml--submode-fontify-region, though, I wonder if maybe
> region extension isn't needed at all, since that function seems to
> handle sub-mode region boundaries.  So I guess that is one
> experiment that could be done.
>
> Anders> One minor mode that, when enabled, would cause Emacs to hang
> Anders> with the above buffer is
> Anders> https://github.com/Lindydancer/char-font-lock (this package
> Anders> highlights incorrect whitespace).
>
> Maybe this mode could be changed instead.


Unfortunately, with the current font-lock interface, it's not. (Well, at
least I haven't figured out a way to do it, and I've been writing Emacs
packages for 25 years.)

The problem is that when the "expand" function is called, the functions has
no way of knowing whether it's the first time it has been called, or if it
has been called repeatedly. Besides, what should it do if there is another
expand function that has adjusted the region in the opposite order? Who has
the right of way?

Anyway, it's not just a problem for this mode, but for all font-lock
packages that expand the region.

    -- Anders


>
> Tom
>

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

bug#41441: 26.3; mhtml misbehaving font-lock extend region can cause Emacs to hang

[Dmitry Gutov]

On 25.05.2020 19:11, Tom Tromey wrote:
> Looking at mhtml--submode-fontify-region, though, I wonder if maybe
> region extension isn't needed at all, since that function seems to
> handle sub-mode region boundaries.  So I guess that is one
> experiment that could be done.

That makes sense to me, and mirrors what mmm-mode currently does.

What if we just remove mhtml--extend-font-lock-region? Can anyone 
describe a scenario that would become broken?

bug#41441: 26.3; mhtml misbehaving font-lock extend region can cause Emacs to hang

[Tom Tromey]

Anders> Hi!To me it's obviously a bug. First, the name of the
Anders> function contains "extend", not "change".

Yes, but the manual says:

   You can enlarge (or even reduce) the region to refontify by setting
the following variable:

See (info "(elisp) Region to Refontify")

Anders> Without having a deep knowledge of this, I don't think this
Anders> do the correct thing when font-lock-beg is at the end of a line
Anders> (as it is when the line is empty). The problem is that the code
Anders> move forward one character (the newline) and then search
Anders> backward with line-beginning-position (i.e. the beginning of the
Anders> line following the original font-lock-beg) as limit, effectively
Anders> shrinking the region.

IIRC this weird thing was needed to correctly find the spot where the
property change really occurred, without having trouble when starting
exactly on the boundary.

I don't remember the test case.  Not sure if the self-tests cover this;
maybe it's archived in a bug somewhere though.

Tom

bug#41441: 26.3; mhtml misbehaving font-lock extend region can cause Emacs to hang

[Tom Tromey]

Dmitry> What if we just remove mhtml--extend-font-lock-region? Can anyone
Dmitry> describe a scenario that would become broken?

Not offhand.  Some manual testing and/or looking through the archived
bugs (or maybe mailing list archives) would be needed.

Tom

bug#41441: 26.3; mhtml misbehaving font-lock extend region can cause Emacs to hang

[Dmitry Gutov]

On 28.05.2020 19:13, Tom Tromey wrote:
> You can enlarge (or even reduce) the region to refontify by setting
> the following variable:

This is about font-lock-extend-after-change-region-function, though, 
isn't it?

And since that variable is not a hook (or a list), it wouldn't be 
subject to inflooping problems. Though I wouldn't like this semantics 
for it either, given the name.

> I don't remember the test case.  Not sure if the self-tests cover this;
> maybe it's archived in a bug somewhere though.

Someone should take a look.

If we don't find good examples, though, and if we agree that the design 
seems sound, we can turn it off on master and see if anyone complains.

bug#41441: 26.3; mhtml misbehaving font-lock extend region can cause Emacs to hang

[Tom Tromey]

>> You can enlarge (or even reduce) the region to refontify by setting
>> the following variable:

Dmitry> This is about font-lock-extend-after-change-region-function, though,
Dmitry> isn't it?

Wow, I totally misread that.  Yeah, I agree.

Dmitry> If we don't find good examples, though, and if we agree that the
Dmitry> design seems sound, we can turn it off on master and see if anyone
Dmitry> complains.

I think it would be better to try it a bit first.

Tom

bug#41441: 26.3; mhtml misbehaving font-lock extend region can cause Emacs to hang

[Anders Lindgren]

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

Hi!

I have given some thought to the other end of the problem (if we should
allow shrinking regions) -- how should font-lock behave when there are
functions that pull in different directions (and it will also handle the
case when a single function returns non-nil without changing the region).

The loop could keep track of which regions it has stared with. When a
functions change the region, the new region it is recorded and the loop
restarts. When the region is resized into something that has already been
seen it doesn't restart. This would allow the region to shrink and grow,
without hanging.

When having conflicting functions, the result is somewhat arbitrary.
Effectively, functions later i the list take precedence over earlier, but
picking one result is better than a hanging Emacs.

    -- Anders


On Thu, May 28, 2020 at 9:15 PM Tom Tromey <tom@tromey.com> wrote:

> >> You can enlarge (or even reduce) the region to refontify by setting
> >> the following variable:
>
> Dmitry> This is about font-lock-extend-after-change-region-function,
> though,
> Dmitry> isn't it?
>
> Wow, I totally misread that.  Yeah, I agree.
>
> Dmitry> If we don't find good examples, though, and if we agree that the
> Dmitry> design seems sound, we can turn it off on master and see if anyone
> Dmitry> complains.
>
> I think it would be better to try it a bit first.
>
> Tom
>

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

bug#41441: 26.3; mhtml misbehaving font-lock extend region can cause Emacs to hang

[Dmitry Gutov]

On 28.05.2020 22:31, Anders Lindgren wrote:
> When having conflicting functions, the result is somewhat arbitrary. 
> Effectively, functions later i the list take precedence over earlier, 
> but picking one result is better than a hanging Emacs.

I wouldn't say a non-deterministic result is much better than hanging.

We should just disallow this kind of behavior in these functions. Unless 
there is some very convincing evidence of its necessity.

bug#41441: 26.3; mhtml misbehaving font-lock extend region can cause Emacs to hang

[Tom Tromey]

Dmitry> Someone should take a look.

Dmitry> If we don't find good examples, though, and if we agree that the
Dmitry> design seems sound, we can turn it off on master and see if anyone
Dmitry> complains.

I tried it out a little on some code I have here, and as far as I can
tell it is ok.

Also see bug #29159, where Stefan says that this function should never
shrink the region -- and where I also speculate that the function is
probably not needed.  Funny.

Soon I will install the patch to remove it.

Tom

bug#41441: 26.3; mhtml misbehaving font-lock extend region can cause Emacs to hang

[Dmitry Gutov]

On 31.05.2020 19:08, Tom Tromey wrote:
> Also see bug #29159, where Stefan says that this function should never
> shrink the region -- and where I also speculate that the function is
> probably not needed.  Funny.

That happens sometimes. :-)

But you seem to be speculating there about whether 
font-lock-extend-region-multiline is needed. Not the mhtml specific 
function.

> Soon I will install the patch to remove it.

Thanks!