Subject: Re: bug#33887: 26.1; Emacs hangs for several seconds when going to the end of an XML file in nXML mode

From: Noam Postavsky <[email-masked]>
Date: Mon, 20 May 2019 16:47:11 -0400

> There's an issue with the following XML file, which does not have
> any special character, except a single quote in the middle of the
> text.
>
> <root>
> <a>12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890
> </a>
> </root>
>
> Note that the newline character before the </a> is important.

Right, this is due to chunking by syntax-propertize.  Here's the fix:

>From 2025fa25f76fd8a2df46fca8807ca386372757d5 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <[email-masked]>
Date: Mon, 20 May 2019 16:04:24 -0400
Subject: [PATCH 1/2] Handle lone quote 500+ characters away from XML tag
 (Bug#33887)

Because syntax-propertize works in small buffer chunks, the rule for
finding quotes which don't contain angle brackets failed to trigger
when the angle bracket was outside of the current chunk.
* lisp/textmodes/sgml-mode.el (sgml-syntax-propertize-rules): Match
quotes on lines with no other angle bracket or quote too (the
syntax-propertize chunk is extended to cover whole lines).
* test/lisp/nxml/nxml-mode-tests.el (nxml-mode-quote-in-long-text):
New test.
---
 lisp/textmodes/sgml-mode.el       |  9 +++++++--
 test/lisp/nxml/nxml-mode-tests.el | 22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/lisp/textmodes/sgml-mode.el b/lisp/textmodes/sgml-mode.el
index 137745fbc1..b555db7b76 100644
--- a/lisp/textmodes/sgml-mode.el
+++ b/lisp/textmodes/sgml-mode.el
@@ -353,8 +353,13 @@ sgml-font-lock-keywords
      ;; the resulting number of calls to syntax-ppss made it too slow
      ;; (bug#33887), so we're now careful to leave alone any pair
      ;; of quotes that doesn't hold a < or > char, which is the vast majority.
-     ("\\([\"']\\)[^<>\"']*[<>\"']"
-      (1 (unless (eq (char-after (match-beginning 1)) (char-before))
+     ;; We also check quotes which are unpaired to end of line,
+     ;; otherwise we miss the case where the quote might "contain" an
+     ;; angle bracket outside of the current syntax-propertize chunk
+     ;; (this relies on `syntax-propertize-wholelines' being enabled).
+     ("\\([\"']\\)[^<>\"']*\\([<>\"']\\|$\\)"
+      (1 (unless (eq (char-after (match-beginning 1))
+                     (char-after (match-beginning 2)))
            ;; Be careful to call `syntax-ppss' on a position before the one
            ;; we're going to change, so as not to need to flush the data we
            ;; just computed.
diff --git a/test/lisp/nxml/nxml-mode-tests.el b/test/lisp/nxml/nxml-mode-tests.el
index 2bbf92bc96..0916a1e652 100644
--- a/test/lisp/nxml/nxml-mode-tests.el
+++ b/test/lisp/nxml/nxml-mode-tests.el
@@ -86,5 +86,27 @@ nxml-mode-tests-correctly-indented-string
     (should (= 1 (car (syntax-ppss (1- (point-max))))))
     (should (= 0 (car (syntax-ppss (point-max)))))))
 
+(ert-deftest nxml-mode-quote-in-long-text ()
+  (with-temp-buffer
+    (nxml-mode)
+    (insert "<t>"
+            ;; `syntax-propertize-wholelines' extends chunk size based
+            ;; on line length, so newlines are significant!
+            (make-string syntax-propertize-chunk-size ?a) "\n"
+            "'"
+            (make-string syntax-propertize-chunk-size ?a) "\n"
+            "</t>")
+    ;; If we just check (syntax-ppss (point-max)) immediately, then
+    ;; we'll end up propertizing the whole buffer in one chunk (so the
+    ;; test is useless).  Simulate something more like what happens
+    ;; when the buffer is viewed normally.
+    (cl-loop for pos from (point-min) to (point-max)
+             by syntax-propertize-chunk-size
+             do (syntax-ppss pos))
+    (syntax-ppss (point-max))
+    ;; Check that last tag is parsed as a tag.
+    (should (= 1 (- (car (syntax-ppss (1- (point-max))))
+                    (car (syntax-ppss (point-max))))))))
+
 (provide 'nxml-mode-tests)
 ;;; nxml-mode-tests.el ends here
-- 
2.11.0

Note that you have to be sure to recompile sgml-mode.el AND nxml-mode.el
after applying these patches, 'make' isn't smart enough to do it
automatically (yes, I figured this out the hard way).

>> <t>>1</t>
>>
>> (syntax-ppss) on the location of "1" in the above, gives (-1 ...).  And
>> then (syntax-ppss) on the "/" will give (0 ...).  So the syntax
>> propertize rule for quote use of (zerop (car (syntax-ppss))) no longer
>> works correctly to see whether it's inside or outside a tag.
>>
>> ">" outside of tags should be set to syntax ".", but I would assume that
>> adding a syntax-propertize rule which calls syntax-ppss for every ">"
>> (to check whether it's inside a tag or not) will be very slow, just like
>> calling it for every quote was.

Oh, I figured it out, we can just look at (nth 9 ppss), because the list
of open parens is still okay, regardless of unmatched close parens.

>From d1520ab5b94d0f130955800ea11222a3702a5519 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <[email-masked]>
Date: Mon, 20 May 2019 16:29:04 -0400
Subject: [PATCH 2/2] Handle ">" outside SGML/XML tags (Bug#33887)

* lisp/textmodes/sgml-mode.el (sgml-syntax-propertize-rules): Check
the list of open parens rather than current depth, the latter is not
reliable.
* test/lisp/textmodes/sgml-mode-tests.el (sgml-tests--quotes-syntax):
Extend test for this case.
---
 lisp/textmodes/sgml-mode.el            | 4 +++-
 test/lisp/textmodes/sgml-mode-tests.el | 9 ++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/lisp/textmodes/sgml-mode.el b/lisp/textmodes/sgml-mode.el
index b555db7b76..052201e5ee 100644
--- a/lisp/textmodes/sgml-mode.el
+++ b/lisp/textmodes/sgml-mode.el
@@ -364,7 +364,9 @@ sgml-font-lock-keywords
            ;; we're going to change, so as not to need to flush the data we
            ;; just computed.
            (let ((ppss (syntax-ppss (match-beginning 0))))
-             (if (prog1 (zerop (car ppss)) ; Outside tag.
+             ;; Can't rely on depth (nth 0 ppss), because we don't
+             ;; mark ">" outside of tags.
+             (if (prog1 (null (nth 9 ppss)) ; Outside tag.
                    (goto-char (1- (match-end 0)))
                    ;; If we're in a comment, don't skip over comment
                    ;; ender.
diff --git a/test/lisp/textmodes/sgml-mode-tests.el b/test/lisp/textmodes/sgml-mode-tests.el
index 09941fe6f1..d6913863d6 100644
--- a/test/lisp/textmodes/sgml-mode-tests.el
+++ b/test/lisp/textmodes/sgml-mode-tests.el
@@ -138,13 +138,16 @@ sgml-with-content
                  "<t>\"a'\"</t>"
                  "<t>'a\"'</t>"
                  "<t><!-- ' --></t>"
-                 "<t><!-- \" --></t>"))
+                 "<t><!-- \" --></t>"
+                 ;; Yes, ">" is technically valid outside tags!
+                 "<t>>'</t>"
+                 ))
     (ert-info (str :prefix "Test string: ")
       (sgml-with-content
        str
        ;; Check that last tag is parsed as a tag.
-       (should (= 1 (car (syntax-ppss (1- (point-max))))))
-       (should (= 0 (car (syntax-ppss (point-max)))))))))
+       (should (= 1 (- (car (syntax-ppss (1- (point-max))))
+                       (car (syntax-ppss (point-max))))))))))
 
 (provide 'sgml-mode-tests)
 ;;; sgml-mode-tests.el ends here
-- 
2.11.0