unterminated strings handled incorrectly

RESOLVED FIXED in mozilla1.8beta2

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla1.8beta2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

13 years ago
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
(Assignee)

Comment 1

13 years ago
Created attachment 177508 [details] [diff] [review]
patch

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

Updated

13 years ago
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [patch]
Target Milestone: --- → mozilla1.8beta2
(Assignee)

Updated

13 years ago
Attachment #177508 - Flags: superreview?(bzbarsky)
Attachment #177508 - Flags: review?(bzbarsky)
Comment on attachment 177508 [details] [diff] [review]
patch

Makes sense.  r+sr=bzbarsky
Attachment #177508 - Flags: superreview?(bzbarsky)
Attachment #177508 - Flags: superreview+
Attachment #177508 - Flags: review?(bzbarsky)
Attachment #177508 - Flags: review+
(Assignee)

Comment 3

13 years ago
Fix checked in to trunk, 2005-03-15 10:58 -0800.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 5

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

Comment 6

13 years ago
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.
(Assignee)

Comment 7

13 years ago
Created attachment 177824 [details] [diff] [review]
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.
(Assignee)

Comment 8

13 years ago
Comment on attachment 177824 [details] [diff] [review]
possible patch

This also fixes the original bug.  Does it fix the regression?
Attachment #177824 - Flags: superreview?(bzbarsky)
Attachment #177824 - Flags: review?(bzbarsky)
(Assignee)

Updated

13 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Comment 9

13 years ago
Created attachment 177827 [details] [diff] [review]
revised patch
Attachment #177824 - Attachment is obsolete: true
Attachment #177827 - Flags: superreview?(bzbarsky)
Attachment #177827 - Flags: review?(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....
Attachment #177827 - Flags: superreview?(bzbarsky)
Attachment #177827 - Flags: superreview+
Attachment #177827 - Flags: review?(bzbarsky)
Attachment #177827 - Flags: review+
Attachment #177824 - Flags: superreview?(bzbarsky)
Attachment #177824 - Flags: superreview-
Attachment #177824 - Flags: review?(bzbarsky)
Attachment #177824 - Flags: review-
(Assignee)

Comment 11

13 years ago
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.
(Assignee)

Comment 12

13 years ago
Fix checked in to trunk, 2005-03-17 22:56 -0800.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.