unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode
@ 2022-01-12  7:20 Colin
  2022-01-12 13:16 ` Eli Zaretskii
  0 siblings, 1 reply; 12+ messages in thread
From: Colin @ 2022-01-12  7:20 UTC (permalink / raw)
  To: 53203

Hi,


I have the following snippet in example.scss file:

```

/*
#ffffff #ffffff
#ffffff #ffffff
#ffffff #ffffff
#ffffff #ffffff
#ffffff #ffffff
*/
```

On opening the file with emacs -Q (27.1 and a fresh build of git master 
(29.0.50), emacs hangs.

If I remove one/two lines, it appears to burn some CPU and then work, so 
it looks like an exponential search or something.

For now I'll remove the snippet from my code, but an interesting bug 
nonetheless ;)








^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode
  2022-01-12  7:20 bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode Colin
@ 2022-01-12 13:16 ` Eli Zaretskii
  2022-01-13  7:00   ` Lars Ingebrigtsen
  0 siblings, 1 reply; 12+ messages in thread
From: Eli Zaretskii @ 2022-01-12 13:16 UTC (permalink / raw)
  To: Colin; +Cc: 53203

> Date: Wed, 12 Jan 2022 17:20:24 +1000
> From: Colin <my.old.email.sucked@gmail.com>
> 
> I have the following snippet in example.scss file:
> 
> ```
> 
> /*
> #ffffff #ffffff
> #ffffff #ffffff
> #ffffff #ffffff
> #ffffff #ffffff
> #ffffff #ffffff
> */
> ```
> 
> On opening the file with emacs -Q (27.1 and a fresh build of git master 
> (29.0.50), emacs hangs.
> 
> If I remove one/two lines, it appears to burn some CPU and then work, so 
> it looks like an exponential search or something.
> 
> For now I'll remove the snippet from my code, but an interesting bug 
> nonetheless ;)

It seems to infloop in JIT font-lock, and the culprit seems to be this
part of font-lock-keywords:

       ;; Even though pseudo-elements should be prefixed by ::, a
       ;; single colon is accepted for backward compatibility.
       "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
                                       css-pseudo-element-ids)
                               t)





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode
  2022-01-12 13:16 ` Eli Zaretskii
@ 2022-01-13  7:00   ` Lars Ingebrigtsen
  2022-01-13  8:58     ` Eli Zaretskii
  2022-05-14 10:34     ` Simen Heggestøyl
  0 siblings, 2 replies; 12+ messages in thread
From: Lars Ingebrigtsen @ 2022-01-13  7:00 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 53203, Colin

Eli Zaretskii <eliz@gnu.org> writes:

> It seems to infloop in JIT font-lock, and the culprit seems to be this
> part of font-lock-keywords:
>
>        ;; Even though pseudo-elements should be prefixed by ::, a
>        ;; single colon is accepted for backward compatibility.
>        "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
>                                        css-pseudo-element-ids)
>                                t)

Trying to understand the regexp used for scss here, I think that bit is
somewhat innocuous -- it just matches those words.

       ;; Even though pseudo-elements should be prefixed by ::, a
       ;; single colon is accepted for backward compatibility.
       "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
                                       css-pseudo-element-ids)
                               t)
       "\\|::" (regexp-opt css-pseudo-element-ids t) "\\)"

But then we get:

       "\\(?:([^)]+)\\)?"
       (if (not sassy)
           "[^:{}()\n]*"
         (concat "[^:{}()\n#]*\\(?:" scss--hash-re "[^:{}()\n#]*\\)*"))
       "\\)*"

Which is a whole lot of backtracking, presumably exacerbated by the
previous ids bit of the regexp.

But I've repressed all I once knew about the scss language -- what is it
really trying to match here?  Anybody?

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





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode
  2022-01-13  7:00   ` Lars Ingebrigtsen
@ 2022-01-13  8:58     ` Eli Zaretskii
  2022-05-14 10:34     ` Simen Heggestøyl
  1 sibling, 0 replies; 12+ messages in thread
From: Eli Zaretskii @ 2022-01-13  8:58 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 53203, my.old.email.sucked

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Colin <my.old.email.sucked@gmail.com>,  53203@debbugs.gnu.org
> Date: Thu, 13 Jan 2022 08:00:18 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It seems to infloop in JIT font-lock, and the culprit seems to be this
> > part of font-lock-keywords:
> >
> >        ;; Even though pseudo-elements should be prefixed by ::, a
> >        ;; single colon is accepted for backward compatibility.
> >        "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
> >                                        css-pseudo-element-ids)
> >                                t)
> 
> Trying to understand the regexp used for scss here, I think that bit is
> somewhat innocuous -- it just matches those words.
> 
>        ;; Even though pseudo-elements should be prefixed by ::, a
>        ;; single colon is accepted for backward compatibility.
>        "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
>                                        css-pseudo-element-ids)
>                                t)
>        "\\|::" (regexp-opt css-pseudo-element-ids t) "\\)"

I posted that because I saw this regexp in the backtrace obtained by
interrupting the infloop.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode
  2022-01-13  7:00   ` Lars Ingebrigtsen
  2022-01-13  8:58     ` Eli Zaretskii
@ 2022-05-14 10:34     ` Simen Heggestøyl
  2022-05-14 12:02       ` Lars Ingebrigtsen
  2022-05-14 12:38       ` Lars Ingebrigtsen
  1 sibling, 2 replies; 12+ messages in thread
From: Simen Heggestøyl @ 2022-05-14 10:34 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Eli Zaretskii, 53203, Colin

Lars Ingebrigtsen <larsi@gnus.org> writes:

> But then we get:
>
>        "\\(?:([^)]+)\\)?"
>        (if (not sassy)
>            "[^:{}()\n]*"
>          (concat "[^:{}()\n#]*\\(?:" scss--hash-re "[^:{}()\n#]*\\)*"))
>        "\\)*"
>
> Which is a whole lot of backtracking, presumably exacerbated by the
> previous ids bit of the regexp.

By the ancient scientific method of commenting out code, it seems to me
that the culprit is rather the last group of the regexp starting on
line 955:

  (concat "\\(?:" scss--hash-re
          "\\|[^@/:{}() \t\n#]\\)"
          "[^:{}()#]*\\(?:" scss--hash-re "[^:{}()#]*\\)*"))

That is, this part:

  \\(?:" scss--hash-re "[^:{}()#]*\\)*

Though I'm rather clueless on how proceed debugging/optimizing it. 😕

> But I've repressed all I once knew about the scss language -- what is
> it really trying to match here?  Anybody?

It's supposed to match selectors like the one on line 39 in
test/manual/indent/scss-mode.scss:

  p.#{$name} var





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode
  2022-05-14 10:34     ` Simen Heggestøyl
@ 2022-05-14 12:02       ` Lars Ingebrigtsen
  2022-05-14 12:38       ` Lars Ingebrigtsen
  1 sibling, 0 replies; 12+ messages in thread
From: Lars Ingebrigtsen @ 2022-05-14 12:02 UTC (permalink / raw)
  To: Simen Heggestøyl; +Cc: Eli Zaretskii, 53203, Colin

Simen Heggestøyl <simenheg@runbox.com> writes:

>> But I've repressed all I once knew about the scss language -- what is
>> it really trying to match here?  Anybody?
>
> It's supposed to match selectors like the one on line 39 in
> test/manual/indent/scss-mode.scss:
>
>   p.#{$name} var

Ah, I see -- it wants to match a selector followed by braces.  I.e.,
these things:

p.#{$name} var
{
}

p.#{$name}, f.#{$bar}:active, f.bar::after {
}

p.#{$name} f.#{$bar} k.var #{$bar}  #{$bar}
{
}

p.#{$name}
{
}

Hm...


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





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode
  2022-05-14 10:34     ` Simen Heggestøyl
  2022-05-14 12:02       ` Lars Ingebrigtsen
@ 2022-05-14 12:38       ` Lars Ingebrigtsen
  2022-05-14 13:30         ` Lars Ingebrigtsen
  1 sibling, 1 reply; 12+ messages in thread
From: Lars Ingebrigtsen @ 2022-05-14 12:38 UTC (permalink / raw)
  To: Simen Heggestøyl; +Cc: Eli Zaretskii, 53203, Colin

OK, I think I understand most of the regexp now, but it's a strange mix
of strictness and sloppiness.

    (,(concat
       "^[ \t]*\\("
       (if (not sassy)
           ;; We don't allow / as first char, so as not to
           ;; take a comment as the beginning of a selector.
           "[^@/:{}() \t\n][^:{}()]*"
         ;; Same as for non-sassy except we do want to allow { and }
         ;; chars in selectors in the case of #{$foo}
         ;; variable interpolation!
         (concat "\\(?:" scss--hash-re
                 "\\|[^@/:{}() \t\n#]\\)"
                 "[^:{}()#]*\\(?:" scss--hash-re "[^:{}()#]*\\)*"))
       ;; Even though pseudo-elements should be prefixed by ::, a
       ;; single colon is accepted for backward compatibility.
       "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
                                       css-pseudo-element-ids)
                               t)
       "\\|::" (regexp-opt css-pseudo-element-ids t) "\\)"
       "\\(?:([^)]+)\\)?"

First we work really hard to see whether we have something that looks
like a selector, i.e. a line that starts with,

p
p::after
p:active

or in the scss case

p#{foo}:active


and then we allow any kind of junk for the rest of the line, like

p.a *[line noise]* p.b {
}

with this bit:

     (if (not sassy)
           "[^:{}()\n]*"
         (concat "[^:{}()\n#]*\\(?:" scss--hash-re "[^:{}()\n#]*\\)*"))
       "\\)*"
       "\\)\\(?:\n[ \t]*\\)*{")
     (1 'css-selector keep))


Anyway, I still don't grok this bit:

         (concat "\\(?:" scss--hash-re
                 "\\|[^@/:{}() \t\n#]\\)"
                 "[^:{}()#]*\\(?:" scss--hash-re "[^:{}()#]*\\)*"))

That is, we're matching an scss/css selector first, and then...
possibly another one?  Which we're doing later, anyway.

So just getting rid of that bit seems to lead to the same highlighting
(and gets rid of the original reported problem).  But like I said, I
don't understand quite what it's trying to do there.  Can anybody come
up with an scss example this breaks?

diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 1139fd1976..fd47a7b83b 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -953,14 +953,13 @@ css--font-lock-keywords
          ;; chars in selectors in the case of #{$foo}
          ;; variable interpolation!
          (concat "\\(?:" scss--hash-re
-                 "\\|[^@/:{}() \t\n#]\\)"
-                 "[^:{}()#]*\\(?:" scss--hash-re "[^:{}()#]*\\)*"))
+                 "\\|[^@/:{}() \t\n][^:{}()]\\)*"))
        ;; Even though pseudo-elements should be prefixed by ::, a
        ;; single colon is accepted for backward compatibility.
        "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
                                        css-pseudo-element-ids)
                                t)
-       "\\|::" (regexp-opt css-pseudo-element-ids t) "\\)"
+       "\\|::" (regexp-opt css-pseudo-element-ids t) "\\)?"
        "\\(?:([^)]+)\\)?"
        (if (not sassy)
            "[^:{}()\n]*"



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






^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode
  2022-05-14 12:38       ` Lars Ingebrigtsen
@ 2022-05-14 13:30         ` Lars Ingebrigtsen
  2022-05-14 15:33           ` Lars Ingebrigtsen
  0 siblings, 1 reply; 12+ messages in thread
From: Lars Ingebrigtsen @ 2022-05-14 13:30 UTC (permalink / raw)
  To: Simen Heggestøyl; +Cc: Eli Zaretskii, 53203, Colin

But how about just trying to make it stricter?  That is, just allow any
number of selectors, with the separators between.  The following seems
to work fine in the test cases.

diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 1139fd1976..4db3ae82f0 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -944,29 +944,29 @@ css--font-lock-keywords
     ;; I use `keep' this should work better).  But really the part of the
     ;; selector between [...] should simply not be highlighted.
     (,(concat
-       "^[ \t]*\\("
+       "^[ \t]*\\(\\(?:"
        (if (not sassy)
            ;; We don't allow / as first char, so as not to
            ;; take a comment as the beginning of a selector.
-           "[^@/:{}() \t\n][^:{}()]*"
+           "[[:alnum:]%*#.>]+"
          ;; Same as for non-sassy except we do want to allow { and }
          ;; chars in selectors in the case of #{$foo}
          ;; variable interpolation!
          (concat "\\(?:" scss--hash-re
-                 "\\|[^@/:{}() \t\n#]\\)"
-                 "[^:{}()#]*\\(?:" scss--hash-re "[^:{}()#]*\\)*"))
+                 "\\|[[:alnum:]%*#.>]+\\)"))
        ;; Even though pseudo-elements should be prefixed by ::, a
        ;; single colon is accepted for backward compatibility.
        "\\(?:\\(:" (regexp-opt (append css-pseudo-class-ids
                                        css-pseudo-element-ids)
                                t)
-       "\\|::" (regexp-opt css-pseudo-element-ids t) "\\)"
+       "\\|::" (regexp-opt css-pseudo-element-ids t) "\\)\\)?"
+       ;; Braces after selectors.
+       "\\(?:\\[[^]]+\\]\\)?"
+       ;; Parentheses after selectors.
        "\\(?:([^)]+)\\)?"
-       (if (not sassy)
-           "[^:{}()\n]*"
-         (concat "[^:{}()\n#]*\\(?:" scss--hash-re "[^:{}()\n#]*\\)*"))
-       "\\)*"
-       "\\)\\(?:\n[ \t]*\\)*{")
+       ;; Separators between selectors.
+       "[ \t,+~>]*"
+       "\\)+\\)\\(?:\n[ \t]*\\)*{")
      (1 'css-selector keep))
     ;; In the above rule, we allow the open-brace to be on some subsequent
     ;; line.  This will only work if we properly mark the intervening text


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






^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode
  2022-05-14 13:30         ` Lars Ingebrigtsen
@ 2022-05-14 15:33           ` Lars Ingebrigtsen
  2022-05-15 10:45             ` Simen Heggestøyl
  0 siblings, 1 reply; 12+ messages in thread
From: Lars Ingebrigtsen @ 2022-05-14 15:33 UTC (permalink / raw)
  To: Simen Heggestøyl; +Cc: Eli Zaretskii, 53203, Colin

Lars Ingebrigtsen <larsi@gnus.org> writes:

> But how about just trying to make it stricter?  That is, just allow any
> number of selectors, with the separators between.  The following seems
> to work fine in the test cases.

I've now done something like this in Emacs 29, and added a bunch of tests.

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





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode
  2022-05-14 15:33           ` Lars Ingebrigtsen
@ 2022-05-15 10:45             ` Simen Heggestøyl
  2022-05-15 12:14               ` Lars Ingebrigtsen
  0 siblings, 1 reply; 12+ messages in thread
From: Simen Heggestøyl @ 2022-05-15 10:45 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Eli Zaretskii, 53203, Colin

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> But how about just trying to make it stricter?  That is, just allow any
>> number of selectors, with the separators between.  The following seems
>> to work fine in the test cases.
>
> I've now done something like this in Emacs 29, and added a bunch of tests.

Hm, I can't seem to find it, did you push it yet?

I tested a bit with the patch you posted 15:30 yesterday, but it seems
to have some problems:

Pasting this into a CSS mode buffer now freezes up Emacs:

  /*
   * xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
   */

Selectors with hyphens are no longer highlighted, e.g.:

  .foo-bar {

Only the last part of multi line selectors are now highlighted,
e.g. only `body` here:

  div,
  body {





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode
  2022-05-15 10:45             ` Simen Heggestøyl
@ 2022-05-15 12:14               ` Lars Ingebrigtsen
  2022-05-15 14:46                 ` Simen Heggestøyl
  0 siblings, 1 reply; 12+ messages in thread
From: Lars Ingebrigtsen @ 2022-05-15 12:14 UTC (permalink / raw)
  To: Simen Heggestøyl; +Cc: Eli Zaretskii, 53203, Colin

Simen Heggestøyl <simenheg@runbox.com> writes:

> Hm, I can't seem to find it, did you push it yet?

Nope; forgot to, and it seems like a good thing.  :-)

> I tested a bit with the patch you posted 15:30 yesterday, but it seems
> to have some problems:
>
> Pasting this into a CSS mode buffer now freezes up Emacs:
>
>   /*
>    * xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>    */
>
> Selectors with hyphens are no longer highlighted, e.g.:
>
>   .foo-bar {
>
> Only the last part of multi line selectors are now highlighted,
> e.g. only `body` here:
>
>   div,
>   body {

I've now fixed these and pushed the change.

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





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode
  2022-05-15 12:14               ` Lars Ingebrigtsen
@ 2022-05-15 14:46                 ` Simen Heggestøyl
  0 siblings, 0 replies; 12+ messages in thread
From: Simen Heggestøyl @ 2022-05-15 14:46 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Eli Zaretskii, 53203, Colin

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Simen Heggestøyl <simenheg@runbox.com> writes:
>
>> I tested a bit with the patch you posted 15:30 yesterday, but it seems
>> to have some problems:
>>
>> Pasting this into a CSS mode buffer now freezes up Emacs:
>>
>>   /*
>>    * xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>>    */
>>
>> Selectors with hyphens are no longer highlighted, e.g.:
>>
>>   .foo-bar {
>>
>> Only the last part of multi line selectors are now highlighted,
>> e.g. only `body` here:
>>
>>   div,
>>   body {
>
> I've now fixed these and pushed the change.

Thanks! Looks good.

The new selector tests makes it much less scary to tweak the regexp.





^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-05-15 14:46 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-12  7:20 bug#53203: Comment with lots of color codes crashes or hangs emacs in scss-mode Colin
2022-01-12 13:16 ` Eli Zaretskii
2022-01-13  7:00   ` Lars Ingebrigtsen
2022-01-13  8:58     ` Eli Zaretskii
2022-05-14 10:34     ` Simen Heggestøyl
2022-05-14 12:02       ` Lars Ingebrigtsen
2022-05-14 12:38       ` Lars Ingebrigtsen
2022-05-14 13:30         ` Lars Ingebrigtsen
2022-05-14 15:33           ` Lars Ingebrigtsen
2022-05-15 10:45             ` Simen Heggestøyl
2022-05-15 12:14               ` Lars Ingebrigtsen
2022-05-15 14:46                 ` Simen Heggestøyl

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).