1360399 - Don't deduplicate revalidation selectors.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez [:emilio]]

It's unfortunate, but it's a correctness issue. I was looking at the expectations update here:

 * https://hg.mozilla.org/integration/autoland/rev/659cddddd434

And investigating it I realised that it's wrong to coalesce selectors like that, because we keep the bloom filter flags.

So in the test cases disabled, we have a selector that looks like this:

msub > :not(:first-child),
msup > :not(:first-child),
msubsup > :not(:first-child),
mmultiscripts > :not(:first-child) {
    -moz-script-level: +1;
    -moz-math-display: inline;
}

And an element that looks like this:

<msubsup><mi></mi><mi></msubsup>

We're only inserting the first selector msub > :not(:first-child) into the set, so when we're going to match the <mi> elements we fast-reject it in both cases due to the bloom filter, so they share style.

I can't see an easy way to fix this keeping the deduplication. If we keep it, we need to remove the bloom filter optimization, which means that we'd trash the cache for every first-child in the document (the :not(:first-child) effectively becomes a global rule).

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1360399: Don't deduplicate revalidation selectors.

It's unfortunate, but it's a correctness issue. I was looking at the
expectations update here:

 * https://hg.mozilla.org/integration/autoland/rev/659cddddd434

And investigating it I realised that it's wrong to coalesce selectors like that,
because we keep the bloom filter flags.

So in the test cases disabled, we have a selector that looks like this:

msub > :not(:first-child),
msup > :not(:first-child),
msubsup > :not(:first-child),
mmultiscripts > :not(:first-child) {
    -moz-script-level: +1;
    -moz-math-display: inline;
}

And an element that looks like this:

<msubsup><mi></mi><mi></mi></msubsup>

We're only inserting the first selector msub > :not(:first-child) into the set,
so when we're going to match the <mi> elements we fast-reject it in both cases
due to the bloom filter, so they share style.

I can't see an easy way to fix this keeping the deduplication. If we keep it, we
need to remove the bloom filter optimization, which means that we'd trash the
cache for every first-child in the document (the :not(:first-child) effectively
becomes a global rule).

Review commit: https://reviewboard.mozilla.org/r/134548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134548/

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1360399: Re-enable tests.

Review commit: https://reviewboard.mozilla.org/r/134550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134550/

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8862678 [details]
Bug 1360399: Don't deduplicate revalidation selectors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134548/diff/1-2/

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8862678 [details]
Bug 1360399: Don't deduplicate revalidation selectors.

https://reviewboard.mozilla.org/r/134548/#review137510

Nice find! Sorry for screwing that up.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8862679 [details]
Bug 1360399: Re-enable tests.

https://reviewboard.mozilla.org/r/134550/#review137514

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8862679 [details]
Bug 1360399: Re-enable tests.

https://reviewboard.mozilla.org/r/134550/#review137516

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Tests were re-enabled (accidentally?) in bug 1351548. Patch at https://github.com/servo/servo/commit/7a556a7f03a64d8cf3d128590bf5bbc66813137f