286262 - unterminated strings handled incorrectly

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

In CSS, unterminated strings are implied to be terminated by a newline, but in
that case they're an error and shouldn't be used.

Steps to reproduce: load
http://fantasai.inkedblade.net/style/tests/ad-hoc/parsing/001

Actual results: the first declaration is sans-serif and blue

Expected results: all declarations should be serif, the first should be blue

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch

(This makes us more compatible with other browsers, IIRC.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177508 [details] [diff] [review]
patch

Makes sense.  r+sr=bzbarsky

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Fix checked in to trunk, 2005-03-15 10:58 -0800.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Actually, I'm not sure the fix is the right one now... The following code (at
http://lxr.mozilla.org/mozilla/source/themes/modern/global/filepicker.css#73 for
now, but soon hopefully to be fixed):

.up-button {
  list-style-image: url("chrome://global/skin/filepicker/folder-up.gif");
  min-width: 0%; // don't let XUL layout min-size override our max-width setting
  min-height: 0%;
  max-width: 36px;
}

causes us to ignore all of the sheet after the '\'', because we're inside
SkipDeclaration, GetToken() returns false (unterminated string), we report what
we think is EOF and bail out, which stops skipping at the newline and returns
from ParseDeclarationBlock, so we start parsing the next declaration block with
the text "min-height: 0%", and it's all bad from there...

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

That seems like a bug in SkipDeclaration or something similar; we'd have had the
same incorrect behavior with a faulty escape, no?

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Well, nsCSSScanner.h's header says that false means error or EOF, but there are
clearly many assumptions that it's an EOF.

Maybe we should create an ERROR token type instead.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: possible patch

Not tested yet.  Creates an error token type instead, which is probably much
more compatible with existing code.  I also removed the unneeded manual
numbering of an enum.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 177824 [details] [diff] [review]
possible patch

This also fixes the original bug.  Does it fix the regression?

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: revised patch

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177827 [details] [diff] [review]
revised patch

r+sr=bzbarsky, though I suspect this will make us report errors twice in some
cases....

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The errors are a little verbose, but it's all in one message, so I think it's
fine.  Too much information is better than too little.

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Fix checked in to trunk, 2005-03-17 22:56 -0800.