876585 - nsCSSScanner::Backup doesn't handle escapes correctly

Status: NEW

(Core :: CSS Parsing and Computation, defect)

Description

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

As I pointed out in bug 543151 comment 81 (which I noticed while reviewing bug 750388), nsCSSScanner::Backup behaves incorrectly when it's being used where escapes are involved.  I don't see an obvious fix here.

I haven't yet managed to find a testcase where we incorrectly accept something that we should reject, but it wouldn't surprise me if such a testcase exists.

Comment 1

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

Attachment: testcase (post-bug 876570)

Comment 2

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

Attachment: testcase (independent of recent/known future changes)

Comment 3

[Zack Weinberg (:zwol)]

Attachment: another testcase

Here's another testcase: Firefox 21 displays alternating green and blue rows, trunk displays all green.  This may be functionally equivalent to attachment 754683 [details] but I'm not sure.

We can fix most of this pretty easily by abandoning the notion of backing up, instead parsing 'b' directly out of the dimension suffix when necessary.  However, when the value of 'b' consists entirely of escaped digits (as in this testcase) I'm not sure whether it's supposed to be accepted.

Comment 4

[Zack Weinberg (:zwol)]

Correction: Firefox 20 is the one that displays alternating green and blue rows on attachment 754835 [details].

Comment 5

[Zack Weinberg (:zwol)]

Attachment: WIP patch

Here's a patch which attempts to handle an+b without backing up at all.  It's not quite right yet: in particular, :nth-child(n-/**/2) is rejected, contra test_selectors.html's expectations.  Also, ExtractInteger is probably reinventing the wheel.  Still, this should be good enough for am-I-barking-up-the-wrong-tree-entirely reactions.

Comment 6

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

We might want to wait on this until the current discussion on www-style about an+b syntax finishes up...

Comment 7

[Zack Weinberg (:zwol)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #6)
> We might want to wait on this until the current discussion on www-style
> about an+b syntax finishes up...

OK, I sent a note to www-style so they know about this wrinkle as well as the other stuff being discussed under that head.

Comment 8

[Benjamin Smedberg]

Resetting owner to default per Zack's request.