265542 - Many invalid selectors are not causing the rule to be ignored

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mats Palmgren (inactive)]

Many invalid selectors are not causing the rule to be ignored.

Testcases and patch coming up...

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 5

[Mats Palmgren (inactive)]

Attachment: Screenshot of JS Console with Patch rev. 1

Comment 6

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

Comment on attachment 162944 [details] [diff] [review]
Patch rev. 1

Why do you need to propagate these funny values further?  (What's the
difference between stopped and ended, dare I ask?)

Can't you just delete list if ParseSelector fails here:

>-    if (! ParseSelector(aErrorCode, selector)) {
>+    if (! ParseSelector(aErrorCode, selector, parsingStatus)) {
>       break;

?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Unfortunately, ParseSelector() will fail there if the thing it tried to parse is
the '{' that starts the declaration block....  I seem to recall running into the
same problem in the context of one of the error-reporting issues timeless filed.

The real issue is that selector parsing is trying to propagate back three
possible states ("ran into something that doesn't look like as selector", "ran
into something that looks like a selector, but isn't" and "parsed selector") via
a two-state boolean return value.  I suspect by judicious ungetting in the ""ran
into something that looks like a selector, but isn't" case we _may_ be able to
trigger the rule to be ignored (simply because the ExpectSymbol('{') will fail),
but that's a lot more fragile than simply propagating back the selector-parsing
state.

That said, I agree that the selector-parsing constant names could be vastly
improved... and maybe they need to become return values?

If we're going for minimally intrusive I agree with mats that this patch is the
way to go, though.

Comment 8

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

Comment on attachment 162944 [details] [diff] [review]
Patch rev. 1

ok, sr=dbaron assuming bz reviews

Comment 9

[Mats Palmgren (inactive)]

Attachment: Testcase #4

Here's a similar bug I found while doing a closer code inspection...

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

(In reply to comment #6)
> Why do you need to propagate these funny values further?  (What's the
> difference between stopped and ended, dare I ask?)

ENDED_OK: "continue parsing the selector"
STOPPED_OK: "we saw a token that implies we are done parsing the selector"
STOPPED_ERROR: we encountered an error

> Can't you just delete list if ParseSelector fails here:

I didn't want to trigger:

  if (!list) {
    REPORT_UNEXPECTED(PESelectorGroupNoSelector);
  }

below the loop. I have now rearranged the code.


(In reply to comment #7)
There are four possible states:
1. We ran into a token (or end-of-file) that means we are done parsing the
   selector.
2. As 1, but the selector was empty (dataMask == 0)
3. We should continue selector parsing.
4. We encountered an error (unexpected token or bad token value or end-of-file
   with an unfinished selector).

> I agree that the selector-parsing constant names could be vastly
> improved... and maybe they need to become return values?
> 

Patch rev. 2 uses return values and these names:

  enum nsParsingStatus {
    eParsingStatus_SelectorDone,     // case 1
    eParsingStatus_SelectorContinue, // case 3
    eParsingStatus_Stop,	     // case 2
    eParsingStatus_Error	     // case 4
  };


This patch also fixes the problem with Testcase #4:
+  // We expect an identifier with a language abbreviation
+  if (eCSSToken_Ident != mToken.mType) {
+    REPORT_UNEXPECTED_TOKEN(PELangArgNotIdent);
+    UngetToken();
+    return eParsingStatus_Error;
+  }

The UngetToken(); was missing.

Comment 11

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -uw)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 163452 [details] [diff] [review]
Patch rev. 2 (diff -uw)

I think it'd make more sense to call it SelectorParsingStatus, not
nsParsingStatus.  It'd also be good to document the statuses where they are
declared.

Past that things look good at first glance.  I'll look in detail if you think
this is ready to go.

Comment 13

[Mats Palmgren (inactive)]

(In reply to comment #12)
How about:

  enum nsSelectorParsingStatus {
    // we have parsed a selector and we saw a token that cannot be part of a
selector:
    eSelectorParsingStatus_Done,
    // we should continue parsing the selector:
    eSelectorParsingStatus_Continue,
    // same as "Done" but we did not find a selector:
    eSelectorParsingStatus_Stop,
    // we saw an unexpected token or token value,
    // or we saw end-of-file with an unfinished selector:
    eSelectorParsingStatus_Error 
  };

Is eSelectorParsingStatus_Empty better than Stop?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yes, eSelectorParsingStatus_Empty is clearer on what's going on.  The rest look
good.

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Comment 16

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3 (diff -uw)

Comment 17

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

Comment on attachment 163471 [details] [diff] [review]
Patch rev. 3

>+  nsSelectorParsingStatus parsingStatus;
>   while (!done) {
>     nsCSSSelector selector;
>+    parsingStatus = ParseSelector(aErrorCode, selector);

Better to declare the variable inside the loop (where it's first assigned to --
that ensures it's never used uninitialized).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 163471 [details] [diff] [review]
Patch rev. 3

With David's comment, r=me.

I suspect this patch fixes bug 261143.	Does it?

Comment 19

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4

Fixed David's comment 17 - no other changes.

Comment 20

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4 (diff -uw)

Comment 21

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

I'm wondering if ParseNegatedSimpleSelector needs to pass aSkipWS as PR_TRUE in
some places, but that's a subject for another bug...

Comment 22

[Mats Palmgren (inactive)]

Checked in 2004-11-26 13:05 PDT

Filed bug 271916 on ParseNegatedSimpleSelector mentioned in comment 21

-> FIXED